Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce galaxy_external_url variable #18576

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Jul 19, 2024

Would fix #18572 (comment),
and prevent further abuse of galaxy_infrastructure_url, with the
downside that users would receive links that don't correspond to their
subdomain server if they had been using one.

The upside is that URLs can change, so maybe this is preferable over
recording the URL for jobs/invocations/histories etc, and it seems
messy to determine in what context one would like to see e.g.
a job finish message.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

mvdbeek added 2 commits July 19, 2024 16:23
Would fix
galaxyproject#18572 (comment),
and prevent further abuse of galaxy_infrastructure_url, with the
downside that users would receive links that don't correspond to their
subdomain server if they had been using one.

The upside is that URLs can change, so maybe this is preferable over
recording the URL for jobs/invocations/histories etc, and it seems
messy to determine in what context one would like to see e.g.
a job finish message.
if job.workflow_invocation_step:
invocation_id_encoded = app.security.encode_id(job.workflow_invocation_step.workflow_invocation_id)
link_invocation = (
f"{app.config.galaxy_infrastructure_url}/workflows/invocations/report?id={invocation_id_encoded}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... well that was problematic 😨. I agree we should have this option but we should just disable this link if the config option is not set IMO. Things should just degrade well if the new option is not set and I think that means either serializing the request url with the job or dropping it all together.

URL (with schema http/https) of the Galaxy instance as accessible
from external networks, including ``galaxy_url_prefix`` if
necessary. This URL is used to determine links outside of the web
application, e.g. when requesting job finish emails.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
application, e.g. when requesting job finish emails.
application, e.g. when requesting job finish emails. This is complementary
to `galaxy_infrastructure_url` which is for internal network use.

within your local network, including ``galaxy_url_prefix`` if
necessary. This URL is used as a default by pulsar file staging
and Interactive Tool containers for communicating back with Galaxy
via the API.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
via the API.
via the API. This setting should be contrasted with `galaxy_external_url` which
is for public facing services.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this redundant with as accessible within your local network and as accessible from external networks and the option name itself ? I think it's good to be concise here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, maybe it can be simplified to See also: galaxy_external_url. The intent was to compare and contrast related settings, so they are not missed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Galaxy behind galaxy_url_prefix returns invalid history link when Email notification is true
3 participants